[17.0][FIX] account_invoice_facturx: fix Factur-X 4.x schematron errors#1
Open
[17.0][FIX] account_invoice_facturx: fix Factur-X 4.x schematron errors#1
Conversation
|
Hi @alexis-via, |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughSeller email Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@account_invoice_facturx/tests/test_facturx_invoice.py`:
- Line 8: The test currently uses assertRaises(UserError) which is too broad;
tighten it to prove the error is raised by the new pre-validation path of
generate_facturx_xml rather than later xml_check_xsd. Replace the generic
assertRaises with an assertRaisesRegex(UserError, "missing.*account") that
matches the exact pre-validation message produced by xml_pre_validate, and/or
patch/mock xml_check_xsd inside the test so it would raise a different sentinel
or assert it was not called; target the generate_facturx_xml() call in
test_facturx_invoice.py and references to xml_pre_validate/xml_check_xsd to
ensure the failure originates from the early guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc16d03d-dcd0-4141-a3dd-7b6cfdeffe8a
📒 Files selected for processing (2)
account_invoice_facturx/models/account_move.pyaccount_invoice_facturx/tests/test_facturx_invoice.py
tendil
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
account_invoice_facturx/tests/test_facturx_invoice.py (1)
160-162:⚠️ Potential issue | 🟡 MinorTighten the missing-account test to prove the pre-validation failure path.
At Line 161,
assertRaises(UserError)is still broad and can pass on downstream failures. Pin it to the explicit missing-account error and/or assert that XSD validation is not reached.Suggested test hardening
+from unittest.mock import patch @@ - with self.assertRaises(UserError): - invoice.generate_facturx_xml() + with patch( + "odoo.addons.account_invoice_facturx.models.account_move.xml_check_xsd" + ) as xml_check_xsd: + with self.assertRaisesRegex( + UserError, "Missing bank account identifier on invoice" + ): + invoice.generate_facturx_xml() + xml_check_xsd.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account_invoice_facturx/tests/test_facturx_invoice.py` around lines 160 - 162, The test currently uses a broad assertRaises(UserError) on invoice.generate_facturx_xml(); tighten it to assert the specific missing-account error (e.g. use self.assertRaisesRegex(UserError, r"missing.*account") when calling invoice.generate_facturx_xml()) and additionally assert that the XSD validation path is not reached by spying/mocking the XSD validation helper (e.g. patch or mock the module/class method used for final validation such as validate_facturx_xml or _validate_xsd and assert it was not called). Ensure you reference invoice.generate_facturx_xml() and the company.write({"facturx_level":"en16931"}) setup in the updated assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@account_invoice_facturx/tests/test_facturx_invoice.py`:
- Around line 160-162: The test currently uses a broad assertRaises(UserError)
on invoice.generate_facturx_xml(); tighten it to assert the specific
missing-account error (e.g. use self.assertRaisesRegex(UserError,
r"missing.*account") when calling invoice.generate_facturx_xml()) and
additionally assert that the XSD validation path is not reached by
spying/mocking the XSD validation helper (e.g. patch or mock the module/class
method used for final validation such as validate_facturx_xml or _validate_xsd
and assert it was not called). Ensure you reference
invoice.generate_facturx_xml() and the
company.write({"facturx_level":"en16931"}) setup in the updated assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a1662e4-de53-4b17-83e7-5b8fbf71fe8f
📒 Files selected for processing (1)
account_invoice_facturx/tests/test_facturx_invoice.py
tendil
force-pushed
the
17.0-t5347-account_invoice_facturx-fix_factur_4_validation
branch
from
de4453e to
a6d2250
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@account_invoice_facturx/tests/test_facturx_invoice.py`:
- Line 81: Redundant reassignment: remove the post-posting line that sets
cls.invoice.partner_bank_id = cls.proprietary_bank in the test (it's already
assigned at invoice creation using cls.proprietary_bank.id), so delete that
statement (or its equivalent) after calling action_post() and run the tests to
confirm no behavior change; reference symbols: partner_bank_id,
cls.proprietary_bank, action_post, and the test_facturx_invoice creation flow
where partner_bank_id is originally set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fee23927-56c0-4fc8-800b-3bca4835fed9
📒 Files selected for processing (1)
account_invoice_facturx/tests/test_facturx_invoice.py
Prevent Factur-X invoices from failing schematron validation with factur-x 4.x. The generated XML must stay compliant with newer validation rules, otherwise valid Odoo invoices cannot be exported as Factur-X documents. This change removes the invalid email URI attribute, exports a proprietary account identifier when IBAN is not available, and updates tests to cover the new compliant output and the missing-account error. Task: 5347
tendil
force-pushed
the
17.0-t5347-account_invoice_facturx-fix_factur_4_validation
branch
from
a6d2250 to
6a436c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@account_invoice_facturx/tests/test_facturx_invoice.py`:
- Around line 159-160: The test currently clears partner_bank_id only via
copy(default={"partner_bank_id": False}) but action_post() can recompute and
restore it; after creating the invoice (variable invoice) and before/after
calling invoice.action_post() explicitly ensure invoice.partner_bank_id is set
to False (i.e., assign False to the partner_bank_id field on the account.move
instance) so the missing-account scenario is preserved; locate the usage around
invoice.copy(...) and invoice.action_post() and add a direct assignment to
partner_bank_id on that invoice instance to clear it again.
- Around line 133-157: Add a complementary test to cover the IBAN branch: create
an XML root using the IBAN fixture (e.g., mirror
test_credit_transfer_uses_proprietary_id_for_non_iban_account but using the IBAN
bank setup such as self.iban_bank), query the same XPath expressions (ram:IBANID
and ram:ProprietaryID using NSMAP), assert that ram:IBANID nodes are present and
ram:IBANID.text equals the expected sanitized IBAN value, and assert that
ram:ProprietaryID is not present for the IBAN case (use the same variables
xml_root, iban_nodes, proprietary_nodes to keep consistency).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f9fa4d5-4cc1-4f41-adbd-f0abfae01ddf
📒 Files selected for processing (2)
account_invoice_facturx/models/account_move.pyaccount_invoice_facturx/tests/test_facturx_invoice.py
Comment on lines
+133
to
+157
| def test_credit_transfer_uses_proprietary_id_for_non_iban_account(self): | ||
| xml_root = self._generate_xml_root(level="en16931") | ||
| proprietary_nodes = xml_root.xpath( | ||
| "//ram:SpecifiedTradeSettlementPaymentMeans/" | ||
| "ram:PayeePartyCreditorFinancialAccount/" | ||
| "ram:ProprietaryID", | ||
| namespaces=NSMAP, | ||
| ) | ||
| iban_nodes = xml_root.xpath( | ||
| "//ram:SpecifiedTradeSettlementPaymentMeans/" | ||
| "ram:PayeePartyCreditorFinancialAccount/" | ||
| "ram:IBANID", | ||
| namespaces=NSMAP, | ||
| ) | ||
|
|
||
| self.assertTrue( | ||
| proprietary_nodes, | ||
| "Expected ProprietaryID for non-IBAN creditor account", | ||
| ) | ||
| self.assertEqual( | ||
| proprietary_nodes[0].text, | ||
| self.proprietary_bank.sanitized_acc_number, | ||
| ) | ||
| self.assertFalse(iban_nodes, "IBANID should not be generated for non-IBAN bank") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a companion regression for the IBAN branch.
This change rewires both sides of the identifier selection, but the new coverage only asserts the non-IBAN path. A small IBAN fixture/assertion would keep ram:IBANID protected too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@account_invoice_facturx/tests/test_facturx_invoice.py` around lines 133 -
157, Add a complementary test to cover the IBAN branch: create an XML root using
the IBAN fixture (e.g., mirror
test_credit_transfer_uses_proprietary_id_for_non_iban_account but using the IBAN
bank setup such as self.iban_bank), query the same XPath expressions (ram:IBANID
and ram:ProprietaryID using NSMAP), assert that ram:IBANID nodes are present and
ram:IBANID.text equals the expected sanitized IBAN value, and assert that
ram:ProprietaryID is not present for the IBAN case (use the same variables
xml_root, iban_nodes, proprietary_nodes to keep consistency).
Comment on lines
+159
to
+160
| invoice = self.invoice.copy(default={"partner_bank_id": False}) | ||
| invoice.action_post() |
There was a problem hiding this comment.
Clear partner_bank_id after posting as well.
action_post() can recompute stored partner_bank_id, so setting it to False only in copy(default=...) does not reliably preserve the missing-account scenario. That makes this regression prone to posting a valid bank back onto the invoice instead of exercising the new early guard.
Suggested tightening
invoice = self.invoice.copy(default={"partner_bank_id": False})
invoice.action_post()
+ invoice.partner_bank_id = FalseBased on learnings: In Odoo 17, partner_bank_id on account.move is a stored computed field (compute='_compute_partner_bank_id'). Calling action_post() can recompute and overwrite the value set at invoice creation time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@account_invoice_facturx/tests/test_facturx_invoice.py` around lines 159 -
160, The test currently clears partner_bank_id only via
copy(default={"partner_bank_id": False}) but action_post() can recompute and
restore it; after creating the invoice (variable invoice) and before/after
calling invoice.action_post() explicitly ensure invoice.partner_bank_id is set
to False (i.e., assign False to the partner_bank_id field on the account.move
instance) so the missing-account scenario is preserved; locate the usage around
invoice.copy(...) and invoice.action_post() and add a direct assignment to
partner_bank_id on that invoice instance to clear it again.
Some invoices already have a delivery/service date on the Odoo side, but this date was not exported to the generated Factur-X XML. As a result, the XML could fail validation because neither a header delivery date nor a billing period was present. This patch adds the delivery date to the header trade delivery block by exporting `ActualDeliverySupplyChainEvent/OccurrenceDateTime`, using the invoice date as the default delivery/service date. The XML node order is also kept XSD-compliant. Task: 5347
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With factur-x 4.x, schematron validation became stricter and the module
started generating EN16931 XML that no longer passed validation for
credit transfer invoices.
Two concrete problems caused that failure:
URIID, although this attribute is not allowed in that context;
a fallback account identifier, so the payment means block failed
BR-CO-27.
As a result, invoices that were previously generated successfully with
factur-x 3.x started failing during Factur-X XML generation with
factur-x 4.x.
This patch fixes the XML generation logic by:
instead of generating XML that is known to be invalid.
The test suite is updated accordingly:
ProprietaryID;
explicitly.
Task: 5347
Summary by CodeRabbit
Bug Fixes
Tests